Skip to content

Fix Mapper::query() method to pass param types. #208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
andrejs wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix Mapper::query() method to pass param types. #208

andrejs wants to merge 4 commits into from

Conversation

@andrejs
Copy link
Contributor

@andrejs andrejs commented Dec 8, 2016

When using custom query, it's not possible to use custom types for query parameters. For example:

$mapper->query(
    'SELECT * FROM user WHERE state IN (?)',
    [
        ['pending', 'active']
    ],
    [
        \Doctrine\DBAL\Connection::PARAM_STR_ARRAY
    ]
);

This pull request fixes the problem by passing custom param types to \Doctrine\DBAL\Connection::executeQuery() via Mapper::query().

@FlipEverything
Copy link
Contributor

FlipEverything commented Dec 12, 2016

Off: The custom query above is just an example, right? Because you don't need to use a custom query for that. You can execute it like this: $mapper->where(['state IN' => ['pending', 'active']]);

On: I can't say anything about the usefullness of passing through the custom param type.

@andrejs
Copy link
Contributor Author

andrejs commented Dec 12, 2016

Yes, it's just an example for the sake of simplicity. Intended usage of \Spot\Mapper::query() is for custom, complex queries, where you might also have typed parameters. Currently using query() you can't specify them, however it's supported directly by DBAL (see \Doctrine\DBAL\Connection::executeQuery). The method just doesn't proxy them. IMHO this is unnecessary limitation of Spot.

@FlipEverything
Copy link
Contributor

FlipEverything commented Dec 12, 2016

I agree!

@tuupola
Copy link
Contributor

tuupola commented Apr 15, 2017

Needs tests, otherwise looks good to me. Valitron bug was already fixed with #217.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

needs tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@andrejs @FlipEverything @tuupola @vlucas